You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[https://nvbugs/7071872][fix] Align disaggregation cancellation fixtures with batch state - #21994
The test fixtures now use BatchState and SampleStateTorch for previous-batch state, and set capturable_guided_decoder to None in the executor fixture. This aligns the fixtures with fields read during previous-batch processing. The diff contains no production code changes.
QA Engineer Review
The patch modifies one test file and tests/integration/test_lists/waives.txt. It removes six waivers for cancellation-boundary, deferred LoRA generation, and overlap-marker tests. The test file does not appear in the searched test-db/ CI or qa/ manual-QA lists. Test results for the current head are not available. Coverage verdict: sufficient for the fixture repair and restored tests.
Per-File QA Perspective
tests/unittest/_torch/disaggregation/test_disagg_idle_cancellation.py: Updates previous-batch fixtures used by cancellation and token-harvest tests. The file does not appear in the searched test-db/ CI or qa/ manual-QA lists.
tests/integration/test_lists/waives.txt: Removes six NVBug 7071872 skip entries for this test file, restoring those cases from waiver status without changing their assertions.
Description
After #20903, previous-batch processing reads BatchState.guided_failures, but the disaggregation cancellation fixtures construct partial SimpleNamespace batch states. Six CPU tests fail with AttributeError before reaching their cancellation and token-harvest assertions.
Use the real BatchState and SampleStateTorch defaults and explicitly set capturable_guided_decoder = None in the executor fixture. Remove the six waivers added by #21963 for NVBug 7071872, restoring these cases to the existing CPU CI selection.
This is an independent fixture repair based directly on main at 11131ecfbe; it addresses the shared failures observed while validating #19396.
Test Coverage
Run with normal native runtime initialization on x86, with GPUs hidden:
Before the fix, at fc41ebd016: 6 failed, 79 passed; all six failures reproduce the waived guided_failures error.
With this patch on 11131ecfbe: 85 passed, no skips.
Pre-commit and git diff --check passed.
Test consolidation: 85 cases before and after, with no new cases, parameter expansion, or weakened assertions. The six restored cases cover context/generation cancellation, already-completed requests, deferred LoRA token harvesting, and the empty-batch overlap marker. The suite remains under the existing l0_cpu.yml directory selection. Local pytest reported 10.10 seconds for the fixed suite; this is not a CI timing measurement.
PR Checklist
Description explains the failure and repair.
Changes follow the coding guidelines and carry DCO sign-off.
Existing regression coverage passes and the six addressed waivers are removed.
Please check this after reviewing the above items as appropriate for this PR.
No actionable comments were generated in the recent review.
Walkthrough
This change updates disaggregation idle-cancellation tests to build previous_batch with BatchState, adds a fixture attribute, and removes six related integration waivers.
Changes
Disaggregation idle-cancellation tests
Layer / File(s)
Summary
BatchState test setup and waiver removal tests/unittest/_torch/disaggregation/test_disagg_idle_cancellation.py, tests/integration/test_lists/waives.txt
Three tests now build previous_batch with BatchState instead of SimpleNamespace. The executor fixture sets capturable_guided_decoder to None. Six waiver entries for disaggregated idle-cancellation tests were removed.
This change only repairs test fixtures and re-enables six previously waived tests. It does not alter production behavior, and no merge-blocking risk remains.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title follows the required NVBugs ID and lowercase type format. It clearly describes the fixture alignment change.
Description check
The description explains the failure and repair, lists relevant test coverage and results, and completes the PR checklist.
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 1 files.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Reviewing files that changed from the base of the PR and between bf410f4 and e4897ba.
Files selected for processing: 1
tests/integration/test_lists/waives.txt
Files with no reviewable changes: 1
tests/integration/test_lists/waives.txt
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
I checked the fixture repair against the production path it unblocks. After #20903, _process_previous_batch reads BatchState.guided_failures / guided_batch_id and PyExecutor.capturable_guided_decoder (py_executor.py:7613-7615, 11917-11924). With the real BatchState defaults plus capturable_guided_decoder = None, _take_capturable_guided_failures returns [], so each of the six restored cases now reaches its own assertions. The six removed lines are exactly the #21963 waives for nvbugs/7071872, and the pre-merge CPU stage still collects the file (l0_cpu.yml:114). Counts: 0 blockers, 0 possibly blocking, 2 required, 0 nice-to-have. The L0 run for bf410f4 (PR_Github #83773) was still queued when I wrote this, so please merge only after the CPU stage has run the six restored cases green.
[Required] PR description: the auto-generated CodeRabbit block says "the tests are not found in the test-db/ CI or qa/ manual-QA lists" and "The file is not found in the test-db/ CI or qa/ manual-QA lists". That is wrong. tests/integration/test_lists/test-db/l0_cpu.yml:114 (- unittest/_torch/disaggregation, the pre-merge CPU stage) collects this cpu_only file. The six cases failed there in L0_MergeRequest_PR #69106 (stage CPU-Generic-x86-2), which is why they were waived, and your own Test Coverage section says the suite stays under the l0_cpu.yml directory selection. Please delete or correct those CodeRabbit lines, so the PR does not claim that the restored cases never run in CI.
The reason will be displayed to describe this comment to others. Learn more.
[Required] This is about the six test_disagg_idle_cancellation.py waivers removed just above. The auto-waive PR #21958 for the same bug (nvbugs/7071872) is still open, and it adds these same six cases again as full:CPU/unittest/_torch/disaggregation/test_disagg_idle_cancellation.py::... SKIP (https://nvbugs/7071872) lines.
Its hunk sits in the full: section of this file (around lines 87-93, next to full:CPU/unittest/scripts/test_subprocess_timeouts.py), which this PR does not touch. The prefixed lines are also different text from the unprefixed ones removed here, so #21958 still applies cleanly after this PR and the duplicate-line check does not catch it. If it is merged later, the six restored cases are skipped again on the CPU stage, the only stage that collects this file (l0_cpu.yml:114). Nothing in CI would flag it, and the bug would look fixed.
Please close #21958 as superseded by #21963 plus this PR, or link it here and ask the bot owner to close it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
sunnyqgg
left a comment
Dev Engineer Review
The test fixtures now use
BatchStateandSampleStateTorchfor previous-batch state, and setcapturable_guided_decodertoNonein the executor fixture. This aligns the fixtures with fields read during previous-batch processing. The diff contains no production code changes.QA Engineer Review
The patch modifies one test file and
tests/integration/test_lists/waives.txt. It removes six waivers for cancellation-boundary, deferred LoRA generation, and overlap-marker tests. The test file does not appear in the searchedtest-db/CI orqa/manual-QA lists. Test results for the current head are not available. Coverage verdict: sufficient for the fixture repair and restored tests.Per-File QA Perspective
tests/unittest/_torch/disaggregation/test_disagg_idle_cancellation.py: Updates previous-batch fixtures used by cancellation and token-harvest tests. The file does not appear in the searchedtest-db/CI orqa/manual-QA lists.tests/integration/test_lists/waives.txt: Removes six NVBug 7071872 skip entries for this test file, restoring those cases from waiver status without changing their assertions.Description
After #20903, previous-batch processing reads
BatchState.guided_failures, but the disaggregation cancellation fixtures construct partialSimpleNamespacebatch states. Six CPU tests fail withAttributeErrorbefore reaching their cancellation and token-harvest assertions.Use the real
BatchStateandSampleStateTorchdefaults and explicitly setcapturable_guided_decoder = Nonein the executor fixture. Remove the six waivers added by #21963 for NVBug 7071872, restoring these cases to the existing CPU CI selection.This is an independent fixture repair based directly on
mainat11131ecfbe; it addresses the shared failures observed while validating #19396.Test Coverage
Run with normal native runtime initialization on x86, with GPUs hidden:
CUDA_VISIBLE_DEVICES='' python3 -m pytest -m cpu_only tests/unittest/_torch/disaggregation/test_disagg_idle_cancellation.py -q --tb=shortfc41ebd016: 6 failed, 79 passed; all six failures reproduce the waivedguided_failureserror.11131ecfbe: 85 passed, no skips.git diff --checkpassed.l0_cpu.ymldirectory selection. Local pytest reported 10.10 seconds for the fixed suite; this is not a CI timing measurement.PR Checklist